-
Notifications
You must be signed in to change notification settings - Fork 11
Fix Callback bugs #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The redefines break paragraph 3.2.4 of the C++14 specification (a subcategory of the ODR - One Definition Rule) in an insidious way. There's no diagnostic required in these cases, which means no warnings, no errors required by the compiler. It's also undefined behavior. There's a conditional compilation of the function call_fn() in the Callback implementation, and in practice, it's inlined in the call() function in the same object file. Depending on the MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL setting you get an extra "ldr r3,[r3, #0]" instruction or not inlined in call(). Without that indirection, calling a nontrivial callback leads to the execution of "bx r3", where r3 contains the address of the ops structure (dynamically dispatched operations), instead of the address contained in the first member of that structure, which would lead to the type-erased function pointer. And because the structure is at an even address, there's a UsageFault when the CPU tries to enter Arm mode, which isn't supported on Cortex-M. When there's a single definition of MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL everything is fine, because there's only one definition of call_fn() and call() - the last function of the two is the only one that exists in practice, in the binary. But when we redefine MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL we get two definitions of the call() function at the same time, breaking paragraph 3.2.4. And remember, no diagnostic required, and this is undefined behavior. So anything goes. On macOS, USBDevice::endpoint_add() is then linked against call() from USBHost.o, which contains the extra "ldr r3,[r3, #0]" instruction. On Windows, it's instead linked against the call() function from USBHostSerial.o, which contains no indirection, and it leads to a crash. These two translation units are compiled with different settings because the redefines are applied to some parts of the library but not all of them. Remember, breaking 3.2.4 is undefined behavior, so it's ok to link this at random. But keeping them equal wouldn't be ok anyway because 3.2.4 would still be broken by the library in combination with the core.
Parts of the library don't know about the default Mbed configuration. If only the redefines are removed, Callback.h is included already in USBHostConf.h, but without any default Mbed configuration, so we get the trivial version of call(). Next, Arduino.h is included, which includes the Mbed configuration indirectly, but the damage has already been done. Over in USBHostSerial.h, USBHostConf.h is included. After that, Callback.h is included in USBHostSerial.h, and we have the correct Mbed configuration in place, but since it's already been included once, the correct version of call() is never actually used. So we get two versions of call() again, which is undefined behavior, and a crash.
Memory usage change @ c409e1f
Click for full report table
Click for full report CSV
|
sebromero
approved these changes
Nov 8, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The redefines break paragraph 3.2.4 of the C++14 specification (a subcategory of
the ODR - One Definition Rule) in an insidious way. There's no diagnostic
required in these cases, which means no warnings, no errors required by the
compiler. It's also undefined behavior. There's a conditional compilation of the
function call_fn() in the Callback implementation, and in practice, it's inlined
in the call() function in the same object file. Depending on the
MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL setting you get an extra "ldr r3,[r3, #0]"
instruction or not inlined in call(). Without that indirection, calling a
nontrivial callback leads to the execution of "bx r3", where r3 contains the
address of the ops structure (dynamically dispatched operations), instead of the
address contained in the first member of that structure, which would lead to the
type-erased function pointer. And because the structure is at an even address,
there's a UsageFault when the CPU tries to enter Arm mode, which isn't supported
on Cortex-M. When there's a single definition of
MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL everything is fine, because there's only
one definition of call_fn() and call() - the last function of the two is the
only one that exists in practice, in the binary. But when we redefine
MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL we get two definitions of the call()
function at the same time, breaking paragraph 3.2.4. And remember, no diagnostic
required, and this is undefined behavior. So anything goes. On macOS,
USBDevice::endpoint_add() is then linked against call() from USBHost.o,
which contains the extra "ldr r3,[r3, #0]" instruction. On Windows, it's instead
linked against the call() function from USBHostSerial.o, which contains no
indirection, and it leads to a crash. These two translation units are compiled
with different settings because the redefines are applied to some parts of the
library but not all of them. Remember, breaking 3.2.4 is undefined behavior, so
it's ok to link this at random. But keeping them equal wouldn't be ok anyway
because 3.2.4 would still be broken by the library in combination with the core.
Parts of the library don't know about the default Mbed configuration. If only
the redefines are removed, Callback.h is included already in USBHostConf.h,
but without any default Mbed configuration, so we get the trivial version of
call(). Next, Arduino.h is included, which includes the Mbed configuration
indirectly, but the damage has already been done. Over in USBHostSerial.h,
USBHostConf.h is included. After that, Callback.h is included in
USBHostSerial.h, and we have the correct Mbed configuration in place, but
since it's already been included once, the correct version of call() is never
actually used. So we get two versions of call() again, which is undefined
behavior, and a crash.